Skip to content

Use "" instead of <> for vendored boost dependency headers.#8128

Merged
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hzeller:feature-20250827-vendored-boost
Aug 27, 2025
Merged

Use "" instead of <> for vendored boost dependency headers.#8128
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hzeller:feature-20250827-vendored-boost

Conversation

@hzeller
Copy link
Collaborator

@hzeller hzeller commented Aug 27, 2025

Boost is a vendored resources (in bazel but also in the way it is installed with etc/DependencyInstaller.sh).

Thus, use the quote instead of angle-bracket includes for them.

@QuantamHD
Copy link
Collaborator

@hzeller will this work when we import into google3

Boost is a vendored resources (in bazel but also in the way
it is installed with etc/DependencyInstaller.sh).

Thus, use the quote instead of angle-bracket includes for them.

Signed-off-by: Henner Zeller <hzeller@google.com>
@hzeller hzeller force-pushed the feature-20250827-vendored-boost branch from 21e11bd to 7a4c936 Compare August 27, 2025 15:44
@hzeller
Copy link
Collaborator Author

hzeller commented Aug 27, 2025

@QuantamHD : don't know yet, but I'll have a look once it is in here, maybe it requires some copybara updates.

Edit: confirmed it will work.

@hzeller hzeller marked this pull request as ready for review August 27, 2025 16:39
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Comment on lines +27 to +31
deps = [
"//src/sta:opensta_lib",
"@boost.container_hash",
"@boost.unordered",
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this change would require changes to the deps. Were we getting these by some non-hermetic means before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple: In the course of fixing the headers I discovered that these deps were missing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the code fail to compile without them then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe there was boost installed on the system, so it was accidentally compiling.

Because the include was with angle-brackets and thus looked as system header, the blaze would also not necessarily do a layering check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid having it look at anything outside the sandbox? This seems like a hole in the hermetic nature.

@maliberty maliberty enabled auto-merge August 27, 2025 18:56
@maliberty maliberty disabled auto-merge August 27, 2025 18:56
@maliberty maliberty merged commit e79e225 into The-OpenROAD-Project:master Aug 27, 2025
10 of 11 checks passed
hzeller added a commit to hzeller/OpenROAD that referenced this pull request Sep 3, 2025
Like The-OpenROAD-Project#8128, but thse went under the radar due to unusual
file suffices.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants